I don't know whether this makes sense, but we need a name for the function dependencies, so I chose a name that wouldn't be affected by Display.

By the way, I'm confused about why we have so many different names or similar display methods for Expr. Maybe we should organize them or name them more clearly

#11782 filed

I thought identifier are the one to be double quoted in your ticket 😕, but it seems you are modifying column

In my opinion, an identifier is an unqualified column. Are they different? 🤔

identifier could includes dot, that is the reason we need double quote to differentiate it from qualified column. Identifier with dot is relatively rare compare to column, so we don't have double quote every where.

In duckdb, they also has double quote for alias name (identifier).

D select t.a from t;
┌───────┐
│   a   │
│ int32 │
├───────┤
│     1 │
└───────┘
D select t.a as t.a from t;
Parser Error: syntax error at or near "."
LINE 1: select t.a as t.a from t;
                       ^
D select t.a as "t.a" from t;
┌───────┐
│  t.a  │
│ int32 │
├───────┤
│     1 │
└───────┘

If we want to introduce dot notation for struct a.b, it is consistent to duckdb if we don't have double quote for column.

https://duckdb.org/docs/sql/data_types/struct.html

I think only adding double quote for identifier that includes dot is probably a better idea.

If we have column that has name includes dot, we display it like qualifier."name.with.dot"

identifier could includes dot, that is the reason we need double quote to differentiate it from qualified column. Identifier with dot is relatively rare compare to column, so we don't have double quote every where.

Agreed. That makes sense to me. A Column comprises multiple identifiers: catalog.schema.table.column. All the parts are identifiers. However, because we don't have an Expr for an Identifier, I can only handle it for Column. Currently, we represent an identifier as an unqualified column.

Ideally, all of the names should be identifiers (e.g., table name, column name, function name, or alias) that should be quoted if needed. This purpose will impact many parts.

I think only adding double quote for identifier that includes dot is probably a better idea.

If we have column that has name includes dot, we display it like qualifier."dot.column"

I believe quoted_flat_name() that will invoke quote_identifier handles this logic well.

This function doesn't always quote the identifier. Only quote it if it contains invalid characters, such as a dot, bracket, or uppercase letter.

I think only adding double quote for identifier that includes dot is probably a better idea.

If we have column that has name includes dot, we display it like qualifier."name.with.dot"

I'm rethinking what you said. I did a test for the function name using the original method (flat_name).

        let schema = Schema::new(vec![
            Field::new("id", DataType::Int32, false),
            Field::new("MAX(id)", DataType::Int32, false),
            Field::new("state", DataType::Utf8, false),
        ]);

        let plan = table_scan(Some("t"), &schema, None)
            .unwrap()
            .filter(col("state").eq(lit("CO")))
            .unwrap()
            .project(vec![col("MAX(id)"), max(col("id"))])
            .unwrap()
            .build()
            .unwrap();

-----
 right: "Projection: t.MAX(id), MAX(t.id)\n  Filter: t.state = Utf8(\"CO\")\n    TableScan: t"

We can distinguish between them easily.
Indeed, maybe we don't need to quote the identifier if it contains a bracket. Only quoting it for the dot might be simpler. 🤔

@jayzhan211 Before I fix other tests, I want to check if this behavior makes sense. (It involves too many tests 😢 ).
Now, we only quote an identifier if it contains the dot. However, some cases like sum(t1.c1) will also be quoted, even if it's a function call. I think it's not worth doing more checking to exclude this kind of case. What do you think?

I think it is not ideal if sum(t1.c1) is quoted 🤔 . I hope the change is as small as possible, so I would prefer to keep function or others Expr remain the same, only identifier with dot is quoted.

We could also hold on and wait for more input from other's about the change of this, given the change of this is not trivial

Maybe instead of modifying Column, we should modify the display_name for Expr, so if we found column inside ScalarFunction, we could skip the double quote anyway. (by something like boolean flag?)

Maybe instead of modifying Column, we should modify the display_name for Expr, so if we found column inside ScalarFunction, we could skip the double quote anyway. (by something like boolean flag?)

I'm not sure if it's that simple 🤔. In my experience, the column might look like this:

Column { qualifier: None, name: "sum(t1.c1)" }

I think it's hard to find a consistent pattern for it because we use many Column::from_name calls to create projections. For example, in